Skip to content

Conversation

christian-byrne
Copy link
Contributor

@christian-byrne christian-byrne commented Oct 6, 2025

Summary

Implemented node resizing functionality for Vue nodes.

2025-10-06.11-03-35_processed_20251006_110410.mp4

Resolves #5675.

Review Focus

ResizeObserver as single source of truth pattern eliminates feedback loops between manual resize and reactive layout updates. Intrinsic content sizing calculation temporarily resets DOM styles to measure natural content dimensions.

graph TD
    A[User Drags Handle] --> B[Direct DOM Style Update]
    B --> C[ResizeObserver Detects Change]
    C --> D[Layout Store Update]
    D --> E[Slot Position Sync]
    
    style A fill:#f9f9f9,stroke:#333,color:#000
    style E fill:#f9f9f9,stroke:#333,color:#000
Loading

┆Issue is synchronized with this Notion page by Unito

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Oct 6, 2025
Copy link

github-actions bot commented Oct 6, 2025

🎭 Playwright Test Results

Some tests failed

⏰ Completed at: 10/07/2025, 10:18:22 PM UTC

📈 Summary

  • Total Tests: 486
  • Passed: 451 ✅
  • Failed: 1 ❌
  • Flaky: 4 ⚠️
  • Skipped: 30 ⏭️

📊 Test Reports by Browser

  • chromium: View Report • ✅ 442 / ❌ 1 / ⚠️ 4 / ⏭️ 30
  • chromium-2x: View Report • ✅ 2 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • chromium-0.5x: View Report • ✅ 1 / ❌ 0 / ⚠️ 0 / ⏭️ 0
  • mobile-chrome: View Report • ✅ 6 / ❌ 0 / ⚠️ 0 / ⏭️ 0

🎉 Click on the links above to view detailed test results for each browser configuration.

Copy link

github-actions bot commented Oct 6, 2025

🎨 Storybook Build Status

Build completed successfully!

⏰ Completed at: 10/07/2025, 10:03:23 PM UTC

🔗 Links


🎉 Your Storybook is ready for review!

Comment on lines +215 to +188
if (!transformState) {
throw new Error(
'TransformState must be provided for node resize functionality'
)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be worth keeping as a console.error to start, then letting useNodeResize degrade gracefully if it's not provided.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This should always exist.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a conflict between your console.error suggestion and @christian-byrne's view that TransformState should always exist. Since this is an internal component with guaranteed injection, I kept the current error throwing approach for fail-fast debugging.

Comment on lines +123 to +128
const stopMoveListen = useEventListener('pointermove', handlePointerMove)
const stopUpListen = useEventListener('pointerup', handlePointerUp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think adding the listeners within function calls might be part of how we're leaking so many.
The handlers are already ignoring events if we're not resizing, so would it be safe to just pull them out and invert some of the start/stop logic?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

adding the listeners within function calls might be part of how we're leaking so many

What does this mean exactly?

The handlers are already ignoring events if we're not resizing, so would it be safe to just pull them out and invert some of the start/stop logic

So have the listeners active across the node lifecycle instead of just during resize? Isn't that worse?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Analyzed this approach but the current implementation is actually optimal - listeners only exist during resize operations (seconds), avoiding unnecessary event processing. useEventListener handles cleanup properly, so no leaks occur.

christian-byrne and others added 7 commits October 7, 2025 14:59
- Add TransformState injection mock for tests
- Mock useNodeResize composable since tests don't need resize functionality
- Fixes test failures after making TransformState required for resize
- Rename .spec.ts to .test.ts to follow tests-ui conventions

🤖 Generated with [Claude Code](https://claude.ai/code)

Co-Authored-By: Claude <[email protected]>
@christian-byrne christian-byrne force-pushed the vue-node/node-resize-v2 branch from f6504f0 to a1f272b Compare October 7, 2025 22:01
@christian-byrne
Copy link
Contributor Author

I'd like to merge this because I have two other features done that rely on it:

  1. Serialize Vue node sizes to workflow
  2. Mid-execution preview component which has a resize button

simula-r
simula-r previously approved these changes Oct 7, 2025
Copy link
Contributor

@simula-r simula-r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@christian-byrne christian-byrne added the New Browser Test Expectations New browser test screenshot should be set by github action label Oct 7, 2025
@christian-byrne christian-byrne merged commit d915792 into main Oct 7, 2025
2 checks passed
@christian-byrne christian-byrne deleted the vue-node/node-resize-v2 branch October 7, 2025 22:53
christian-byrne added a commit that referenced this pull request Oct 8, 2025
## Summary

Implemented node resizing functionality for Vue nodes.


https://github.com/user-attachments/assets/a7536045-1fa5-401b-8d18-7c26b4dfbfc3

Resolves #5675.

## Review Focus

ResizeObserver as single source of truth pattern eliminates feedback
loops between manual resize and reactive layout updates. Intrinsic
content sizing calculation temporarily resets DOM styles to measure
natural content dimensions.

```mermaid
graph TD
    A[User Drags Handle] --> B[Direct DOM Style Update]
    B --> C[ResizeObserver Detects Change]
    C --> D[Layout Store Update]
    D --> E[Slot Position Sync]
    
    style A fill:#f9f9f9,stroke:#333,color:#000
    style E fill:#f9f9f9,stroke:#333,color:#000
```

┆Issue is synchronized with this [Notion
page](https://www.notion.so/PR-5936-make-Vue-nodes-resizable-2846d73d36508160b3b9db49ad8b273e)
by [Unito](https://www.unito.io)

---------

Co-authored-by: Claude <[email protected]>
Co-authored-by: DrJKL <[email protected]>
Co-authored-by: github-actions <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:node-interaction area:vue-migration New Browser Test Expectations New browser test screenshot should be set by github action size:L This PR changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can't resize the node size
3 participants